skyrl-train/skyrl_train/entrypoints/main_base.py (133)

The application hardcodes trust_remote_code=True when initializing the tokenizer. Combined with the newly added support for cloud storage paths (S3, GCS), this allows for Remote Code Execution (RCE) if an attacker can influence the model path configuration. An attacker could point the application to a malicious remote model containing executable code in its configuration files, which would be downloaded and executed on the training worker. It is recommended to make trust_remote_code a configurable parameter that defaults to False.

skyrl-train/skyrl_train/workers/fsdp/fsdp_worker.py (119)

The application hardcodes trust_remote_code=True when loading model configurations. This is particularly dangerous as this PR enables loading models from arbitrary cloud storage paths (S3, GCS). If an attacker can control the model_path, they can trigger the execution of arbitrary code embedded in the remote model's configuration. This setting should be configurable and default to False to prevent unauthorized code execution.

skyrl-train/skyrl_train/workers/fsdp/fsdp_worker.py (282)

Hardcoded trust_remote_code=True allows for arbitrary code execution when loading models from the provided path. With the addition of cloud path support, this risk is extended to remote sources. This should be made configurable and default to False.

skyrl-train/skyrl_train/workers/fsdp/fsdp_worker.py (357)

Hardcoded trust_remote_code=True allows for arbitrary code execution when loading models from the provided path. With the addition of cloud path support, this risk is extended to remote sources. This should be made configurable and default to False.

skyrl-train/skyrl_train/workers/megatron/megatron_worker.py (431)

The init_model method now supports loading models from cloud storage paths via io.local_read_dir. However, the underlying init_configs method hardcodes trust_remote_code=True when loading the tokenizer and config. This allows a remote attacker who can influence the model_path to achieve Remote Code Execution (RCE) by providing a malicious model. You should avoid hardcoding trust_remote_code=True and instead make it a user-controlled option that defaults to False.

skyrl-train/skyrl_train/workers/megatron/megatron_worker.py (719)

The init_model method now supports loading models from cloud storage paths via io.local_read_dir. However, the underlying init_configs method hardcodes trust_remote_code=True when loading the tokenizer and config. This allows a remote attacker who can influence the model_path to achieve Remote Code Execution (RCE) by providing a malicious model. You should avoid hardcoding trust_remote_code=True and instead make it a user-controlled option that defaults to False.

skyrl-train/skyrl_train/inference_engines/sglang/sglang_engine.py (254-258)

This manual handling of the context manager's __enter__ and __exit__ methods is not robust. If an exception occurs during the initialization of the Engine (line 283), the __exit__ method on line 288 will not be called, leading to a resource leak (the temporary directory will not be cleaned up).

To ensure cleanup happens reliably, you should wrap the code between __enter__ and __exit__ in a try...finally block.

Example:

self._cloud_model_ctx = None
if io.is_cloud_path(original_model_path):
    self._cloud_model_ctx = io.local_read_dir(original_model_path)
    kwargs["model_path"] = self._cloud_model_ctx.__enter__()

try:
    # ... existing initialization code ...
    self.engine = Engine(**kwargs)
    # ...
finally:
    if self._cloud_model_ctx is not None:
        self._cloud_model_ctx.__exit__(None, None, None)
        self._cloud_model_ctx = None

Since this change would affect lines not in the diff, I'm leaving it as a comment, but this is an important fix for resource management.

skyrl-train/skyrl_train/inference_engines/vllm/vllm_engine.py (81-87)

Similar to the sglang_engine.py, the manual use of __enter__ and __exit__ can lead to resource leaks. If self._create_engine() on line 102 raises an exception, the temporary directory for the cloud model will not be cleaned up.

Please wrap the engine initialization logic in a try...finally block to ensure self._cloud_model_ctx.__exit__() is always called after __enter__().

Example:

self._cloud_model_ctx = None
if io.is_cloud_path(original_model_path):
    self._cloud_model_ctx = io.local_read_dir(original_model_path)
    kwargs["model"] = self._cloud_model_ctx.__enter__()
    # ...

try:
    # ... existing initialization code ...
    self.llm = self._create_engine(*args, **kwargs)
finally:
    if self._cloud_model_ctx is not None:
        self._cloud_model_ctx.__exit__(None, None, None)
        self._cloud_model_ctx = None

skyrl-train/skyrl_train/workers/megatron/megatron_worker.py (202-208)

This block of code, which overrides token IDs in the model configuration based on the tokenizer, has been removed. This change seems unrelated to supporting cloud model paths and is not mentioned in the pull request description.

Could you please clarify the reason for this removal? If this logic was necessary to ensure consistency between the tokenizer and the model config, removing it might introduce subtle bugs. If it's now redundant, a brief explanation would be helpful for future maintainers.